Skip to content

feat: bump beats and replace global loggers #16717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

feat: bump beats and replace global loggers #16717

wants to merge 15 commits into from

Conversation

kruskall
Copy link
Member

Motivation/summary

bump beats
fix compile errors
propagare logger and remove global loggers
use logptest in tests

Checklist

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

Related issues

Blocked by elastic/elastic-agent-libs#312

@kruskall
feat: bump beats and replace global loggers
fa191dc 
bump beats
fix compile errors
propagare logger and remove global loggers
use logptest in tests
@kruskall kruskall requested a review from a team as a code owner April 24, 2025 17:58
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 24, 2025

This pull request does not have a backport label. Could you fix it @kruskall? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.17 is the label to automatically backport to the 7.17 branch.
  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • backport-9./d is the label to automatically backport to the 9./d branch. /d is the digit.
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

1pkg
1pkg reviewed Apr 28, 2025
View reviewed changes
Copy link
Member

@1pkg 1pkg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this test is missing from the update.

simitt
simitt reviewed Apr 29, 2025
View reviewed changes
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question regarding the removed cleanup, otherwise LGTM

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 29, 2025

This pull request is now in conflicts. Could you fix it @kruskall? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feat/remove-global-logger upstream/feat/remove-global-logger
git merge upstream/main
git push upstream feat/remove-global-logger

@kruskall kruskall requested review from 1pkg and simitt April 30, 2025 16:14
lahsivjar
lahsivjar reviewed May 2, 2025
View reviewed changes
Copy link
Contributor

@lahsivjar lahsivjar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just one query

internal/beatcmd/testcmd.go
@@ -54,7 +55,7 @@ var testConfigCommand = &cobra.Command{
if cfg.Output.Name() == "elasticsearch" {
esOutputConfig = cfg.Output.Config()
}
if _, err := beaterconfig.NewConfig(cfg.APMServer, esOutputConfig); err != nil {
if _, err := beaterconfig.NewConfig(cfg.APMServer, esOutputConfig, logp.NewLogger("")); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the testing logger here too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is not test code

@kruskall kruskall requested a review from a team May 2, 2025 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lahsivjar lahsivjar lahsivjar left review comments

@1pkg 1pkg 1pkg approved these changes

@rubvs rubvs rubvs approved these changes

@simitt simitt Awaiting requested review from simitt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kruskall @lahsivjar @simitt @1pkg @rubvs